-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a new APNs configuration option push_with_badge
to suppress sending aps.badge
in APNs messages.
#358
base: main
Are you sure you want to change the base?
Conversation
# # | ||
# # Specifies whether to send the badge key in the APNs message. | ||
# # Defaults to True, set this to False to suppress sending the badge count. | ||
# #push_with_badge: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# # | |
# # Specifies whether to send the badge key in the APNs message. | |
# # Defaults to True, set this to False to suppress sending the badge count. | |
# #push_with_badge: false | |
# # | |
# # Specifies whether to update the notification count badge on the app upon sending | |
# # an APNs message (if notification counts can be calculated). Specifically, | |
# # whether or not the `aps.badge` field is set. | |
# # | |
# # If this is False, additional `unread_count` and `missed_calls` fields will be added to | |
# # the APNS message. | |
# # | |
# # Defaults to True, set this to False to suppress sending the badge count. | |
# #push_with_badge: false |
Some slight wording adjustments. You may also want to explain why this could be desirable when "not using mutable-content
"?
Though after writing this out, I feel like we're conflating two different behaviours in a single config option:
- Suppression of
aps.badge
field. - Inclusion of additional
unread_count
andmissed_calls
fields.
Could you say why you want the unread_count
and missed_calls
fields? Or even what the overall goal for this PR is with respect to your implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this comes off as a bit confusing.
The primary focus of this commit to suppress the aps.badge
field so iOS does not automatically draw the badge on the home screen. In the event an iOS app is receiving APNS from multiple independent sources, the badge value is incorrect. Whether the other source is sending its own aps.badge
or not, sygnal sending its own aps.badge
still conflicts with the true badge value expected by the user.
When suppressing that field, I wanted to ensure the underlying data was still available to the iOS client, so it could be used, if desired. The inclusion of the unread_count
and missed_calls
fields were to represent the expanded form of what aps.badge
represents here .
I agree the inclusion of unread_count
and missed_calls
could become their own setting, but I was unsure about APNS message length assumptions.
- If length assumptions are not a concern, it seems like always including
unread_count
andmissed_calls
in APNS would be reasonable (I can submit a separate PR). Then this PR would simply be about toggling the inclusion ofaps.badge
. - If length assumptions are a concern, I'm open to opinions of how to move forward. We can drop the extra fields entirely, or add multiple settings controlling the inclusion of fields. But neither of these options feel very elegant.
@@ -551,14 +552,20 @@ def _get_payload_full( | |||
if loc_args: | |||
payload["aps"].setdefault("alert", {})["loc-args"] = loc_args | |||
|
|||
if badge is not None: | |||
if self.get_config("push_with_badge", bool, True) and badge is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than pulling this option value out of the config on each request, please do so in __init__
and set to a class variable, then reference that class variable instead. i.e.:
def __init__(self, name: str, sygnal: "Sygnal", config: Dict[str, Any]) -> None:
# ...
self.push_with_badge = self.get_config("push_with_badge", bool, True)
def _get_payload_event_id_only(
self, n: Notification, default_payload: Dict[str, Any]
) -> Dict[str, Any]:
# ...
if self.push_with_badge and badge is not None:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 will be in the next commit
…ding `aps.badge` in APNs messages. Signed-off-by: Chris Ennis <[email protected]>
608dee5
to
12c0b05
Compare
For situations when not using
mutable-content
and updating the badge count is not desired.This also adds
unread_count
andmissed_calls
to the push payload whenpush_with_badge
isFalse
.